Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#5569 Add compliance with Trusted Types #5575

Merged
merged 5 commits into from
Jan 24, 2024
Merged

#5569 Add compliance with Trusted Types #5575

merged 5 commits into from
Jan 24, 2024

Conversation

sosnovsky
Copy link
Collaborator

This PR adds Trusted Types policy for compatibility with upcoming Gmail policy update

close #5569

Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@sosnovsky sosnovsky requested a review from martgil January 22, 2024 13:02
@sosnovsky
Copy link
Collaborator Author

@martgil this PR is ready for review, thanks!

martgil
martgil requested changes Jan 23, 2024
View reviewed changes
Copy link
Collaborator

@martgil martgil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is also a good idea to update some of the few options for the DOMPurify configuration option to return trusted type by adding the RETURN_TRUSTED_TYPE property set to true when DOMPurify.sanitize() is used:

flowcrypt-browser/extension/types/purify.d.ts

Line 18 in a403966

export declare function sanitize(source: string | Node, config: Config & { RETURN_DOM: true; }): HTMLElement;

Reference:
https://github.com/cure53/DOMPurify?tab=readme-ov-file#what-about-dompurify-and-trusted-types
https://web.dev/articles/trusted-types#use_a_library

@sosnovsky
Copy link
Collaborator Author

I think it is also a good idea to update some of the few options for the DOMPurify configuration option to return trusted type by adding the RETURN_TRUSTED_TYPE property set to true when DOMPurify.sanitize() is used:

Nice suggestion, I'll check this property and also standard dompurify types from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/dompurify as we currently use outdated types for v2.

@sosnovsky sosnovsky requested a review from martgil January 23, 2024 21:34
@sosnovsky sosnovsky mentioned this pull request Jan 23, 2024
Open
@sosnovsky
Copy link
Collaborator Author

@martgil I tried to add RETURN_TRUSTED_TYPE property to DOMPurify, but it'll require a lot of changes in our code - at least all usages of Xss.htmlSanitize, as currently it returns string, but with Trusted Types it's return type will be string | TrustedHTML. And also adding @types/trusted-types leads to build error, will need investigation too.

So let's plan this change to the next milestone (I created separate issue for it - #5576), as we need to finish current release in the beginning of February, before Gmail policy update.
In this PR I've also updated purify.d.ts to the latest version from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/dompurify

@sosnovsky sosnovsky enabled auto-merge (squash) January 23, 2024 21:40
@martgil
Copy link
Collaborator

martgil commented Jan 24, 2024

@martgil I tried to add RETURN_TRUSTED_TYPE property to DOMPurify, but it'll require a lot of changes in our code - at least all usages of Xss.htmlSanitize, as currently it returns string, but with Trusted Types it's return type will be string | TrustedHTML. And also adding @types/trusted-types leads to build error, will need investigation too.

I understood completely. This PR also looks good. thanks!

So let's plan this change to the next milestone (I created separate issue for it - #5576), as we need to finish current release in the beginning of February, before Gmail policy update. In this PR I've also updated purify.d.ts to the latest version from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/dompurify

Okay, thanks! I'll assigned it to myself to work on it.

martgil
martgil approved these changes Jan 24, 2024
View reviewed changes
Copy link
Collaborator

@martgil martgil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! thank you.

@sosnovsky sosnovsky merged commit 7dbae7d into master Jan 24, 2024
14 checks passed
@sosnovsky sosnovsky deleted the 5569-trusted-types-compliance-live-test branch January 24, 2024 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martgil martgil martgil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check compliance with Trusted Types
2 participants
@sosnovsky @martgil